-
Notifications
You must be signed in to change notification settings - Fork 405
feat(gnovm): Rewrite AST prior go type checking to avoid inter-realm type check errors. #4156
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
🛠 PR Checks SummaryAll Automated Checks passed. ✅ Manual Checks (for Reviewers):
Read More🤖 This bot helps streamline PR reviews by verifying automated checks and providing guidance for contributors and reviewers. ✅ Automated Checks (for Contributors):No automated checks match this pull request. ☑️ Contributor Actions:
☑️ Reviewer Actions:
📚 Resources:Debug
|
gnovm/pkg/gnolang/gotypecheck.go
Outdated
if len(n.Args) > 0 { | ||
a = n.Args[0] | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nitpick: len of n.Args can only be 1, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. I should probably validate the syntax of cross()
and crossing()
while they are still available for check!
|
||
// Validate Gno syntax and type check. | ||
if err := gno.TypeCheckMemPackageTest(memPkg, m.Store); err != nil { | ||
return runResult{Error: err.Error()} | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For my understanding, this is a signal that we’re no longer adding new features to the gno type checker, right?
Do we already have a clear clarification on this plan?
I think what we need to do is review the existing issues related to the gno type checker—either mark them appropriately or close them.
There are also some related PRs, such as #4033. Should we go ahead and merge them? @thehowl @mvertes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From what jae said, we should go through the PRs and issues relating to the type-checker and label them. I think we can close them for now, then recover them if we want to go ahead and improve our own preprocessor / type checker.
Longer term, I hope that we can migrate away from the GnoVM sooner than we consider removing our reliance on the go type checlker.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's going to take a long time to remove the go type checker. At least we should keep it around for fuzz testing against our own, with fuzz tools that can generate any valid code to run against both go and gno.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good to me.
d0b79f5
to
3b5ac5b
Compare
No description provided.